LOC-7325: stop uncatchable TypeError on empty binary output in Local.start - #182
LOC-7325: stop uncatchable TypeError on empty binary output in Local.start#182vivianludrick wants to merge 2 commits into
Conversation
…start
`start()` handles the binary's output inside an `execFile` callback. The
empty-output branch called back with 'No output received' but did not
return, so control fell through to `data['message']['message']` on
`data = {}`. That threw a TypeError, and because the throw happens inside
a callback invoked by node's internal exithandler, no try/catch around
`local.start(...)` could intercept it — it surfaced as an
uncaughtException in the host process.
Three paths reached the same unguarded deref:
- empty stdout and stderr (the reported one) — now returns after the
callback, so it fires exactly once
- the terminal branch of the `error` handler, which also fell through
- any non-connected payload with no `message` key
Also guards `JSON.parse`: non-JSON output threw a SyntaxError from the
same uncatchable position, and is now reported through the callback with
the raw output attached as `extra`.
`startSync` shared the unguarded deref and now uses the same helper. Its
empty-output branch already returned, so it was not exposed to the
fall-through.
Adds regression tests driving start() with stub binaries for each output
shape, asserting the callback fires exactly once and nothing escapes as
an uncaughtException. They need no credentials or network. Three of the
four fail on master with the TypeError from the ticket.
Claude Code ReviewVerdict: the fix works for the output shapes it targets, but it doesn't yet guarantee the PR's stated invariant ("callback fires exactly once and nothing escapes"). Three confirmed gaps in 10 findings survived adversarial verification (7 confirmed by live execution or code inspection, 3 plausible), ranked by severity: Confirmed — code
Confirmed — tests (
|
Addresses all 10 review findings on PR #182: - Shared parseBinaryOutput helper used by both start and startSync: guards JSON.parse in the sync path too (no more binary delete + 9 re-downloads on non-JSON output) and rejects payloads that parse to null or a non-object ('null' is valid JSON, so the parse guard alone missed it). - Once-guarded safeCallback + whole-body try/catch inside the execFile callback so no branch can throw uncatchably or fire the callback twice; consumer-callback throws still propagate. - fs.unlinkSync in both retry branches wrapped: a missing binary no longer aborts the retry. - getErrorMessage only returns strings; non-string payload messages fall back to the generic message. - Exhausted-retry branch parses stdout and prefers the binary's JSON diagnostic over the generic execution error. - Raw output attached as error.extra truncated to 1KB. - Tests: retriesLeft=0 (no accidental real-binary download in CI), deterministic settle instead of fixed 1s sleep, exception-safe uncaughtException snapshot/restore in beforeEach/afterEach, tmpdir cleanup in after(), plus new cases for null output, non-string message, non-zero-exit diagnostics, truncation, and startSync. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Fixes an uncatchable
TypeErrorthrown out ofLocal.start()when the BrowserStackLocal binary exits with no output.JIRA Story: https://browserstack.atlassian.net/browse/LOC-7325
The bug
start()handles the binary's output inside anexecFilecallback. The empty-output branch called back withNo output receivedbut did notreturn, so control fell through to the next statement, which dereferencesdata['message']['message']ondata = {}:Two things make this worse than a normal error path:
No output received, then again from the throwing statement.exithandler, so notry/catcharoundlocal.start(...)intercepts it. It surfaces as anuncaughtException, which means the blast radius is set by the host process's exception policy, not by this package. In the case that surfaced it, a host with a fataluncaughtExceptionhandler lost its entire reporting plane because an optional tunnel failed to start.The trigger is not exotic — any environment where the binary exits without emitting JSON reaches it: wrong or blocked binary path, killed process, permission failure, or a shimmed binary in CI.
The fix
Three paths reached the same unguarded deref; all three are now closed:
returnerrorhandlerreturnmessagekeyFailed to start BrowserStack LocalAlso guarded
JSON.parse: non-JSON output (a plain-text crash message, for instance) threw aSyntaxErrorfrom the same uncatchable position. It is now reported through the callback asInvalid output received: <reason>, with the raw output attached as the error'sextrafield.startSyncshared the unguarded deref and now uses the same helper. Its empty-output branch already returned, so it was never exposed to the fall-through.Every changed path now invokes the callback exactly once and lets the caller handle the failure normally.
Tests
Added
test/local_start_output_handling.js— drivesstart()with stub binaries for each output shape and asserts the callback fires exactly once and that nothing escapes as anuncaughtException. No credentials or network needed.Verified the tests actually catch the defect by toggling the fix:
master: 3 of the 4 fail, with the ticket's exactTypeError: Cannot read properties of undefined (reading 'message').Full suite, excluding the
LocalBinary > Downloadblock that needs real credentials:master: 28 passing, 3 failing, 2 pendingSame 3 failures before and after (
should return is running properly×2,should stop local) — all pre-existing and credential-gated, none related to this change.npm run pretest(eslint overlib/* index.js) is clean.Note: the fix avoids optional chaining because the repo's eslint config sets
env: es6(ES2015).Scope
Code fix only — no version bump or publish here.
1.5.13is the latest published version and carries the defect, so this needs a release to reach consumers.